-
Notifications
You must be signed in to change notification settings - Fork 4
Automated Template Builds, Proxmox User ACLs, Improved LDAP Integration #189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
009f4cb to
edee54e
Compare
| const req = https.get(url, { headers }, (res) => { | ||
| // Handle redirects | ||
| if (res.statusCode === 301 || res.statusCode === 302 || res.statusCode === 307 || res.statusCode === 308) { | ||
| const location = res.headers.location; | ||
| if (!location) { | ||
| return reject(new Error(`Redirect without Location header (status ${res.statusCode})`)); | ||
| } | ||
| // Follow redirect (without auth headers for CDN) | ||
| return httpGet(location, {}, redirectCount + 1) | ||
| .then(resolve) | ||
| .catch(reject); | ||
| } | ||
|
|
||
| let data = ''; | ||
| res.on('data', chunk => data += chunk); | ||
| res.on('end', () => { | ||
| if (timedOut) return; | ||
| resolve({ statusCode: res.statusCode, headers: res.headers, body: data }); | ||
| }); | ||
| }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
General approach: prevent user input from freely controlling the hostname (and redirect targets) used in outgoing HTTPS requests. For a Docker registry helper, the safest fix is to restrict which registry hosts are allowed and optionally block clearly unsafe ones (e.g., localhost, private IPs), and similarly restrict redirects so they cannot jump to arbitrary or internal hosts.
Best concrete fix with minimal functional change:
- Add an allow‑list and host/IP validation logic in
create-a-container/utils/docker-registry.js. - Before performing any registry request in
getImageConfig, validate theregistryvalue (derived from user input) against this policy. - In
httpGet, restrict redirects: only follow redirects that stay on the same host as the original request (or, if needed, also on the same allow‑listed set). This stops an attacker from leveraging redirects to pivot to internal services.
Specific changes:
- In
docker-registry.js, require Node’surlmodule to easily parse hosts for the redirect logic. - Define helper functions, e.g.
isPrivateHostname,isPrivateIp,isHostAllowed, andvalidateRegistryHost, near the top of the file. - Modify
httpGetto:- Parse the original URL’s hostname.
- When following a redirect, resolve the
Locationagainst the original URL and only follow it if the resulting hostname is allowed (same host, or passesisHostAllowed).
- In
getImageConfig, callvalidateRegistryHost(registryHost)before constructing any URLs, rejecting disallowed registry names (e.g.,localhost,.local, private subnets, unless explicitly allow‑listed).
We only touch docker-registry.js; containers.js already normalizes the image format and doesn’t need logic changes for this SSRF mitigation.
-
Copy modified line R10 -
Copy modified lines R12-R20 -
Copy modified lines R22-R85 -
Copy modified lines R105-R115 -
Copy modified line R117 -
Copy modified lines R328-R330
| @@ -7,8 +7,82 @@ | ||
| */ | ||
|
|
||
| const https = require('https'); | ||
| const urlModule = require('url'); | ||
|
|
||
| // Restrictive allow-list of registry hosts that this service is allowed to contact. | ||
| // Extend this list as needed for your deployment. | ||
| const ALLOWED_REGISTRY_HOSTS = new Set([ | ||
| 'docker.io', | ||
| 'registry-1.docker.io', | ||
| 'ghcr.io', | ||
| 'quay.io' | ||
| ]); | ||
|
|
||
| /** | ||
| * Check whether a hostname is clearly unsafe for SSRF (localhost, local domain, etc.) | ||
| * This is a defense-in-depth check; the primary control is the allow-list above. | ||
| * @param {string} hostname | ||
| * @returns {boolean} | ||
| */ | ||
| function isPrivateHostname(hostname) { | ||
| if (!hostname) return true; | ||
| const lower = hostname.toLowerCase(); | ||
| return ( | ||
| lower === 'localhost' || | ||
| lower === '127.0.0.1' || | ||
| lower === '::1' || | ||
| lower.endsWith('.local') || | ||
| lower.endsWith('.localhost') | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Basic check for private IP address ranges. | ||
| * Note: this does not perform DNS resolution; it only checks literal IPs. | ||
| * @param {string} hostname | ||
| * @returns {boolean} | ||
| */ | ||
| function isPrivateIp(hostname) { | ||
| const ipRegex = /^(?:\d{1,3}\.){3}\d{1,3}$/; | ||
| if (!ipRegex.test(hostname)) return false; | ||
| const octets = hostname.split('.').map(Number); | ||
| // 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 | ||
| if (octets[0] === 10) return true; | ||
| if (octets[0] === 172 && octets[1] >= 16 && octets[1] <= 31) return true; | ||
| if (octets[0] === 192 && octets[1] === 168) return true; | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Determine if a host is allowed according to our policy. | ||
| * @param {string} host | ||
| * @returns {boolean} | ||
| */ | ||
| function isHostAllowed(host) { | ||
| if (!host) return false; | ||
| const hostname = host.split(':')[0]; // strip port if present | ||
| if (ALLOWED_REGISTRY_HOSTS.has(hostname)) { | ||
| return true; | ||
| } | ||
| if (isPrivateHostname(hostname) || isPrivateIp(hostname)) { | ||
| return false; | ||
| } | ||
| // By default, disallow unknown hosts to avoid SSRF. | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Validate a registry hostname before using it in outbound requests. | ||
| * Throws if the host is not allowed. | ||
| * @param {string} registryHost | ||
| */ | ||
| function validateRegistryHost(registryHost) { | ||
| if (!isHostAllowed(registryHost)) { | ||
| throw new Error(`Registry host '${registryHost}' is not allowed`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Low-level HTTP GET that returns status, headers, and body without throwing on 4xx | ||
| * @param {string} url - The URL to fetch | ||
| * @param {object} headers - Optional request headers | ||
| @@ -30,8 +102,19 @@ | ||
| if (!location) { | ||
| return reject(new Error(`Redirect without Location header (status ${res.statusCode})`)); | ||
| } | ||
|
|
||
| // Resolve the redirect URL against the original URL to get an absolute URL | ||
| const resolved = new urlModule.URL(location, url); | ||
| const redirectHost = resolved.hostname; | ||
| const originalHost = new urlModule.URL(url).hostname; | ||
|
|
||
| // Only allow redirects that stay on the same host and that host is allowed | ||
| if (redirectHost !== originalHost || !isHostAllowed(redirectHost)) { | ||
| return reject(new Error(`Disallowed redirect from ${originalHost} to ${redirectHost}`)); | ||
| } | ||
|
|
||
| // Follow redirect (without auth headers for CDN) | ||
| return httpGet(location, {}, redirectCount + 1) | ||
| return httpGet(resolved.toString(), {}, redirectCount + 1) | ||
| .then(resolve) | ||
| .catch(reject); | ||
| } | ||
| @@ -243,6 +325,9 @@ | ||
| */ | ||
| async function getImageConfig(registry, repo, tag) { | ||
| const registryHost = registry === 'docker.io' ? 'registry-1.docker.io' : registry; | ||
|
|
||
| // Validate the registry host to avoid SSRF via user-controlled image references | ||
| validateRegistryHost(registryHost); | ||
|
|
||
| // First, fetch the manifest to get the config digest | ||
| const acceptHeaders = { |
|
Talked with Robert a bit and suggested adding some core packages to the Dockerfile, whether it be anything from tmux, jq, git, etc. Items that are commonly used to save time. |
No description provided.